docs(drive): clarify download command is for Workspace native formats, not binary files#836
Conversation
…, not binary files
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the user experience for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
🦋 Changeset detectedLatest commit: cbf175d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
googleworkspace-bot
added
the
area: core
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds clarifying help text to the gws drive files export command to help users distinguish it from the get command when downloading binary files, and includes a corresponding unit test and changeset. The reviewer feedback suggests simplifying the implementation by leveraging the existing method.id field (e.g., 'drive.files.export') to identify the target command, which avoids modifying the signature of build_resource_command and passing the service_name parameter recursively.
| for name in resource_names { | ||
| let resource = &doc.resources[name]; | ||
| if let Some(cmd) = build_resource_command(name, resource) { | ||
| if let Some(cmd) = build_resource_command(&doc.name, name, resource) { |
There was a problem hiding this comment.
Revert passing &doc.name to build_resource_command since we can use method.id directly to identify the target command.
| if let Some(cmd) = build_resource_command(&doc.name, name, resource) { | |
| if let Some(cmd) = build_resource_command(name, resource) { |
References
- Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.
| /// | ||
| /// `service_name` is the top-level API service (e.g. `"drive"`) used to inject | ||
| /// service-specific help text into selected commands. | ||
| fn build_resource_command(service_name: &str, name: &str, resource: &RestResource) -> Option<Command> { |
There was a problem hiding this comment.
Revert the signature of build_resource_command to avoid passing the unused service_name parameter.
fn build_resource_command(name: &str, resource: &RestResource) -> Option<Command> {References
- Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.
|
|
||
| // Inject service-specific after_help notes for commands that are | ||
| // commonly confused with similar-sounding operations. | ||
| if service_name == "drive" && name == "files" && method_name == "export" { |
There was a problem hiding this comment.
Instead of passing service_name through the recursive call stack and performing ad-hoc string matching on service_name, name, and method_name, you can leverage the unique method identifier method.id (e.g., "drive.files.export") which is already provided by the Discovery Document schema.
Using method.id simplifies the condition and allows you to completely revert the signature changes to build_resource_command (removing the service_name parameter and its propagation in build_cli and the recursive call).
| if service_name == "drive" && name == "files" && method_name == "export" { | |
| if method.id.as_deref() == Some("drive.files.export") { |
References
- Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.
| for sub_name in sub_names { | ||
| let sub_resource = &resource.resources[sub_name]; | ||
| if let Some(sub_cmd) = build_resource_command(sub_name, sub_resource) { | ||
| if let Some(sub_cmd) = build_resource_command(service_name, sub_name, sub_resource) { |
There was a problem hiding this comment.
Revert passing service_name recursively.
| if let Some(sub_cmd) = build_resource_command(service_name, sub_name, sub_resource) { | |
| if let Some(sub_cmd) = build_resource_command(sub_name, sub_resource) { |
References
- Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.
2 participants
Description
The
gws drive files exportcommand is designed for exporting Google Workspace native formats (Docs → PDF, Sheets → CSV, etc.). Using it on binary files (PDFs, images, Office documents already stored as binary blobs) returns a 500backendError, which was confusing users.Added a clarifying
NOTEsection to theexportcommand's--helpoutput that:gws drive files get --params '{"alt": "media", ...}') for binary file downloadsThe note is injected in
build_resource_commandincommands.rswhenservice == "drive",resource == "files", andmethod == "export", keeping all service-specific help text co-located with command construction rather than spread across helper files.A new unit test
test_drive_files_export_after_helpverifies the note appears in the generated command'safter_helptext.Closes #727
Checklist:
google-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings.pnpx changeset) to document my changes.